Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read server response on DATA command #190

Closed
wants to merge 1 commit into from

Conversation

kayrus
Copy link
Contributor

@kayrus kayrus commented Jun 3, 2022

This is a braking change. Now Data() method will accept a callaback function and LMTPData() method callbacks will receive success statuses instead nil. Therefore dependent LMTPData code must check for status response code.

Resolves #189

P.S. I rewrote the (d *dataCloser) Close() code to make it more readable.

@kayrus kayrus force-pushed the handle-data-resp branch from bee900f to 35de77e Compare June 3, 2022 19:24
@Slugger
Copy link

Slugger commented Jun 12, 2022

I came here with the exact same dilemma -- I need to capture & log the 250 response to all DATA commands. I was about to write a PR myself then saw this. 😄

The semantics of returning an SMTPError on a 250 response seems a little off to me since this isn't really an error condition. My thought on this was to add some kind of additional property to the client struct that was a string called DataResponse or something like that. It just holds the response to the last DATA command issued (when not an error, errors would still be returned as usual). This would also avoid the breaking change to Close() as discussed.

But I'll gladly take this as well. Any indication of whether this is going to be accepted?

@kayrus
Copy link
Contributor Author

kayrus commented Jun 13, 2022

@Slugger then I can simply add the same functionality as a callback function in LMTP session.

@kayrus kayrus force-pushed the handle-data-resp branch from 35de77e to 0383a63 Compare June 13, 2022 11:21
@kayrus
Copy link
Contributor Author

kayrus commented Jun 13, 2022

@Slugger @emersion I updated the PR, let me know your feedback.

@kayrus kayrus force-pushed the handle-data-resp branch from 0383a63 to 48b3194 Compare June 13, 2022 14:50
@Slugger
Copy link

Slugger commented Jun 13, 2022

I like this approach better for sure. Now I can handle the DATA response as needed via a callback.

@kayrus kayrus force-pushed the handle-data-resp branch from 17b0610 to 48b3194 Compare June 14, 2022 17:08
@kayrus
Copy link
Contributor Author

kayrus commented Jul 5, 2022

@emersion kindly ping

@emersion
Copy link
Owner

emersion commented Feb 3, 2024

Sorry for the delay. I think in general I'm not a huge fan of callbacks... Could we instead turn the returned io.WriteCloser into something like a *DataCommand, and expose the info message as a method?

@foxcpp
Copy link
Collaborator

foxcpp commented Feb 5, 2024

Agreed with emersion, I think adding an explicit method looks like better design to me.

I am also not sure if SMTPError is the right type to use here, I would at least create an alias named SMTPResponse or something - some careful refactoring may be needed here.

@emersion
Copy link
Owner

emersion commented Feb 5, 2024

Do we really need to expose the full response? Is there anything interesting in there apart from the human-readable message?

@Slugger
Copy link

Slugger commented Feb 5, 2024

Usually a message ID from the upstream receiver. My use case at the time was that I was trying to use this as a piece in an email forwarding service and when I forwarded messages upstream I wanted to capture the message id from the receiver for logging, tracking and auditing purposes.

@emersion
Copy link
Owner

Closing in favor of #263

@emersion emersion closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client: read server response on DATA command
4 participants